Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Moves getUserDisplayName outside from meteor app #35031

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

refactor: Moves getUserDisplayName outside from meteor app #35031

wants to merge 2 commits into from

Conversation

dougfabris
Copy link
Member

@dougfabris dougfabris commented Jan 24, 2025 •
edited
Loading

Proposed changes (including videos or screenshots)

It moves the auxiliary getUserDisplayName function to core-typings and useUserDisplayName hook to ui-client in order to give more flexibility to use them outside from meteor app

Issue(s)

Steps to test or reproduce

Further comments

@dionisio-bot dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Jan 24, 2025 •
edited
Loading

Looks like this PR is not ready to merge, because of the following issues:

  • This PR has conflicts, please resolve them before merging
  • This PR is not mergeable
  • This PR is targeting the wrong base branch. It should target 7.4.0, but it targets 7.3.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 435f43e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dougfabris dougfabris mentioned this pull request Jan 24, 2025
Merged
@dougfabris dougfabris added this to the 7.4.0 milestone Jan 24, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 24, 2025 •
edited
Loading

PR Preview Action v1.6.0

🚀 View preview at
https://RocketChat.github.io/Rocket.Chat/pr-preview/pr-35031/

Built to branch gh-pages at 2025-01-24 19:15 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@codecov Codecov
Copy link

codecov bot commented Jan 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.17%. Comparing base (bb5b08d) to head (435f43e).
Report is 22 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #35031   +/-   ##
========================================
  Coverage    59.17%   59.17%           
========================================
  Files         2822     2823    +1     
  Lines        68116    68109    -7     
  Branches     15144    15140    -4     
========================================
- Hits         40310    40306    -4     
+ Misses       24975    24973    -2     
+ Partials      2831     2830    -1
Flag Coverage Δ
unit 75.01% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@dougfabris dougfabris changed the title chore: Moves getUserDisplayName outside from meteor app refactor: Moves getUserDisplayName outside from meteor app Jan 24, 2025
@dougfabris dougfabris force-pushed the chore/move-getUserDisplayName branch from 1c4c436 to 31ed52f Compare January 24, 2025 18:56
@dougfabris dougfabris force-pushed the chore/move-getUserDisplayName branch from 31ed52f to 435f43e Compare January 24, 2025 18:58
@dougfabris dougfabris marked this pull request as ready for review January 24, 2025 21:17
@dougfabris dougfabris requested review from a team as code owners January 24, 2025 21:17
@gabriellsh gabriellsh added the stat: QA assured Means it has been tested and approved by a company insider label Jan 27, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jan 27, 2025
MarcosSpessatto
MarcosSpessatto requested changes Jan 27, 2025
View reviewed changes
packages/core-typings/src/IUser.ts
@@ -264,3 +264,6 @@ export type AvatarServiceObject = {
};

export type AvatarObject = AvatarReset | AvatarUrlObj | FormData | AvatarServiceObject;

export const getUserDisplayName = (name: IUser['name'], username: IUser['username'], useRealName: boolean): string | undefined =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we tested the hook already, but can we add a unit test for this?

@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Jan 30, 2025
@kodiakhq kodiakhq
Copy link
Contributor

kodiakhq bot commented Jan 30, 2025

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcosSpessatto MarcosSpessatto MarcosSpessatto requested changes

@gabriellsh gabriellsh gabriellsh approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
stat: conflict stat: QA assured Means it has been tested and approved by a company insider
Projects
None yet
Milestone
7.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dougfabris @MarcosSpessatto @gabriellsh